Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make EKO into a separate resource #2087

Merged
merged 5 commits into from
May 30, 2024
Merged

Make EKO into a separate resource #2087

merged 5 commits into from
May 30, 2024

Conversation

scarlehoff
Copy link
Member

@scarlehoff scarlehoff commented May 22, 2024 •
edited
Loading

Closes #2081

It can be tested with theory_40001002

From an user perspective there should be no differences. When you only need the fktables / theory (i.e., for reports or fits, when doing check_theoryID) only the fktables will be downloaded.
If the eko needs to be downloaded as well (only for evolution), then it will be downloaded into the same folder as the theory (done in this way so that all the theories that have the eko inside work just the same).

There's a separate check_eko(thid) which is used by evolven3fit and returns the path to the eko.tar of the theory, checking first the theory.

At the moment the ekos range between the 1.5 and 3.5 GB while the whole set of fktables used for 4.0 is about 600 MB so this can have a huge impact e.g. when doing reports in your laptop when the PDF has been generated elsewhere.

RoyStegeman
RoyStegeman approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is dislike a bit about this is that as part of my workflow I will often (manually) run vp-setupfit before submitting many jobs to make sure that all the parallel jobs won't try to download the same thing. This PR means I'll have to be more careful about it and find some way to download the ekos as well.

n3fit/src/evolven3fit/evolve.py Outdated Show resolved Hide resolved
validphys2/src/validphys/loader.py Show resolved Hide resolved
RoyStegeman
RoyStegeman requested changes May 23, 2024
View reviewed changes
Copy link
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no I just see evolven3fit is not on top of reportengine so there is no way to do --no-net which I think is import to prevent the issue of accidentally trying to download the same thing many times.

@scarlehoff
Copy link
Member Author

scarlehoff commented May 23, 2024 •
edited
Loading

Ah no I just see evolven3fit is not on top of reportengine so there is no way to do --no-net which I think is import to prevent the issue of accidentally trying to download the same thing many times.

I can add a --no-net option to evolven3fit if you want.
However, would you like me to add (in that case) a check_eko option to setupfit so that it downloads also the eko? So we can consider setupfit as a "download and check everything that I will need for the fit". n3fit will not check the eko.

Putting evolven3fit on top of validphys would fix quite a few issues but I'm certainly not doing that (in the short term)

@RoyStegeman
Copy link
Member

I can add a --no-net option to evolven3fit if you want.

Yes, thanks.

However, would you like me to add (in that case) a check_eko option to setupfit so that it downloads also the eko?

I was thinking about asking this, but if we want to do an MHOU regression test that will rely on vp-setupfit to compute the covmat. Not sure what the limit for the CI is, would it be fine if it just downloads the eko along with theoryid from the theory key and not theoryids from the theorycovmatconfig (or whatever that key is called)?

@scarlehoff
Copy link
Member Author

would it be fine if it just downloads the eko along with theoryid from the theory key and not theoryids from the theorycovmatconfig (or whatever that key is called)?

Yes. I think it also makes sense since only the eko for the fit's theoryid is needed for setting up the fit.

@RoyStegeman
Copy link
Member

Yes. I think it also makes sense since only the eko for the fit's theoryid is needed for setting up the fit.

Of course, but it immediatly adds 1.5GB more to the CI so I was just asking/wondering how much wiggle room we have.

Anyway, downloading the eko with vp-setupfit would be my preferred solution

@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label May 23, 2024
@scarlehoff
Copy link
Member Author

Done. Let's see what does the fitbot think.

@scarlehoff
Copy link
Member Author

"ok, everything works ok but it would be nice to add a nice small printout. FML" :)

@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels May 23, 2024
@github-actions GitHub Actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label May 28, 2024
@scarlehoff
Copy link
Member Author

Can I merge this @RoyStegeman / are you happy with the setupfit solution or do you want to have a second look?

@scarlehoff scarlehoff merged commit b67498b into master May 30, 2024
6 checks passed
@scarlehoff scarlehoff deleted the eko_as_resource branch May 30, 2024 09:50
@scarlehoff scarlehoff mentioned this pull request Jun 5, 2024
Open
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RoyStegeman RoyStegeman RoyStegeman approved these changes

@comane comane Awaiting requested review from comane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Separate eko.tar and fastkernel tables in the server
2 participants
@scarlehoff @RoyStegeman